Do not block on sync controller start and engage existing clusters #100
Conversation
kcp-ci-bot
added
kind/bug
|
/retest looks like a flake. |
| func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reconcile.Result, error) { | ||
| log := r.log.Named(ControllerName) | ||
| log.Debug("Processing") | ||
| log.Debugw("Processing", "request", req) |
There was a problem hiding this comment.
This controller only ever reconciles a dummy object, it has no meaning to log this dummy value.
| } | ||
|
|
||
| log.Infof("storing worker at %s", key) | ||
| log.Infow("Storing worker", "key", key) |
There was a problem hiding this comment.
I think we can remove this log line, I think it#s a debug leftover from my youth.
| } | ||
|
|
||
| log.Infow("Starting new sync controller…", "key", key) | ||
| log.Infow("Creating new sync controller…", "key", key, "name", pubRes.Name) |
There was a problem hiding this comment.
While we're at it, let's create a prlog := log.With("key", key, "name", pubRes.name) and then use prlog throughout this loop.
On-behalf-of: SAP <marvin.beckers@sap.com> Signed-off-by: Marvin Beckers <marvin@kubermatic.com>
embik
force-pushed
the
multiple-resources
branch
from
4c8ae62 to
4711cef
Compare
|
/retest looks like kcp being slow (probably too many tests running right now). |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: xrstf The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
LGTM label has been added. Git tree hash: 6f51659a8ab0d8ad2d02e8097dc8d296d213811a
|
kcp-ci-bot
added
the
approved
|
/cherry-pick release-0.4 |
|
@embik: once the present PR merges, I will cherry-pick it on top of release-0.4 in a new PR and assign it to you. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/retest Pretty, please? |
|
@embik: new pull request created: #102 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Summary
The migration to multicluster-runtime (#72) left us with a big regression, only a single
PublishedResource's objects were actively synced.Turns out, we are blocking the reconcile loop for the sync manager on the first actual sync controller we start, so no further sync controllers could be started. This PR addresses that problem.
In addition, we found a design flaw in the sync manager: Since we start controllers dynamically, an
Engagecall by a provider might not happen when a dynamic (sync) controller is already running. As such, we retrofitted dynamic engaging of existing clusters on new sync controllers to make sure we bring them up to par with any existing controllers.What Type of PR Is This?
/kind bug
/kind regression
Related Issue(s)
Fixes #99
Release Notes